fix(start-server-core): return 404 for API routes without GET handler #5758
Conversation
WalkthroughNormalize server route handler keys to uppercase, map HEAD to GET when a GET handler exists, consult an ANY fallback, and return early with 404 responses when no handler is found. Adds unit tests, test mocks, and Vite aliases pointing to those mocks for testing. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as createStartHandler
participant Lookup as Handler Lookup (normalized)
participant HandlerFn as Selected Handler
participant Response
Client->>Server: HTTP Request (method, path)
Server->>Lookup: Normalize handlers keys → UPPERCASE
Server->>Lookup: Lookup by method
alt method is HEAD and GET exists
Lookup-->>Server: select GET handler
else no method match -> try ANY
Lookup-->>Server: select ANY handler (if present)
end
alt Handler found
Server->>HandlerFn: invoke handler
HandlerFn-->>Response: handler response
Response-->>Client: handler-defined status/body
else No handler
alt request method is HEAD
Server->>Response: 404 with empty JSON content-type
else
Server->>Response: 404 with {"error":"Not Found"} JSON body
end
Response-->>Client: 404
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/start-server-core/src/createStartHandler.ts(1 hunks)packages/start-server-core/tests/createStartHandler.test.ts(1 hunks)packages/start-server-core/tests/mocks/injected-head-scripts.ts(1 hunks)packages/start-server-core/tests/mocks/router-entry.ts(1 hunks)packages/start-server-core/tests/mocks/start-entry.ts(1 hunks)packages/start-server-core/tests/mocks/start-manifest.ts(1 hunks)packages/start-server-core/vite.config.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/start-server-core/tests/mocks/injected-head-scripts.tspackages/start-server-core/tests/mocks/start-entry.tspackages/start-server-core/tests/mocks/router-entry.tspackages/start-server-core/tests/createStartHandler.test.tspackages/start-server-core/tests/mocks/start-manifest.tspackages/start-server-core/src/createStartHandler.tspackages/start-server-core/vite.config.ts
packages/{*-start,start-*}/**
📄 CodeRabbit inference engine (AGENTS.md)
Name and place Start framework packages under packages/-start/ or packages/start-/
Files:
packages/start-server-core/tests/mocks/injected-head-scripts.tspackages/start-server-core/tests/mocks/start-entry.tspackages/start-server-core/tests/mocks/router-entry.tspackages/start-server-core/tests/createStartHandler.test.tspackages/start-server-core/tests/mocks/start-manifest.tspackages/start-server-core/src/createStartHandler.tspackages/start-server-core/vite.config.ts
🧠 Learnings (4)
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
packages/start-server-core/tests/mocks/router-entry.tspackages/start-server-core/tests/createStartHandler.test.tspackages/start-server-core/tests/mocks/start-manifest.ts
📚 Learning: 2025-09-23T17:36:12.598Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.598Z
Learning: Applies to packages/router-core/** : Keep framework-agnostic core router logic in packages/router-core/
Applied to files:
packages/start-server-core/tests/mocks/router-entry.ts
📚 Learning: 2025-11-02T16:16:24.898Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Applied to files:
packages/start-server-core/tests/mocks/router-entry.tspackages/start-server-core/tests/mocks/start-manifest.tspackages/start-server-core/vite.config.ts
📚 Learning: 2025-09-23T17:36:12.598Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.598Z
Learning: Applies to packages/{router-cli,router-generator,router-plugin,virtual-file-routes}/** : Keep CLI, generators, bundler plugins, and virtual file routing utilities in their dedicated tooling package directories
Applied to files:
packages/start-server-core/vite.config.ts
🧬 Code graph analysis (2)
packages/start-server-core/tests/createStartHandler.test.ts (2)
packages/start-server-core/src/createStartHandler.ts (1)
createStartHandler(54-341)packages/start-server-core/tests/mocks/router-entry.ts (1)
currentHandlers(3-3)
packages/start-server-core/src/createStartHandler.ts (1)
packages/start-client-core/src/serverRoute.ts (1)
RouteMethod(427-435)
🪛 ESLint
packages/start-server-core/tests/mocks/router-entry.ts
[error] 1-1: All imports in the declaration are only used as types. Use import type.
(@typescript-eslint/consistent-type-imports)
[error] 3-3: 'currentHandlers' is never reassigned. Use 'const' instead.
(prefer-const)
packages/start-server-core/tests/createStartHandler.test.ts
[error] 1-1: Member 'expect' of the import declaration should be sorted alphabetically.
(sort-imports)
packages/start-server-core/vite.config.ts
[error] 7-7: Expected 1 empty line after import statement not followed by another import.
(import/newline-after-import)
[error] 7-7: path import should occur before import of vitest/config
(import/order)
[error] 7-7: Prefer node:path over path.
(node/prefer-node-protocol)
🔇 Additional comments (9)
packages/start-server-core/tests/mocks/start-manifest.ts (1)
1-10: LGTM!The mock manifest structure is minimal and appropriate for testing the server handler behavior.
packages/start-server-core/tests/mocks/start-entry.ts (1)
1-7: LGTM!The mock start entry provides minimal defaults suitable for testing the start handler.
packages/start-server-core/tests/mocks/injected-head-scripts.ts (1)
1-1: LGTM!The empty string mock for injected head scripts is appropriate for isolated testing.
packages/start-server-core/src/createStartHandler.ts (2)
383-392: LGTM! Correct HTTP method normalization and HEAD mapping.The normalization ensures case-insensitive method matching, and the HEAD→GET mapping follows HTTP semantics correctly.
395-408: LGTM! Proper 404 handling for routes without matching handlers.The logic correctly returns 404 JSON responses when no handler exists for the requested method and no ANY fallback is present. HEAD requests appropriately return an empty body with 404 status per HTTP standards.
packages/start-server-core/vite.config.ts (1)
15-23: LGTM! Test mock aliases properly configured.The path aliases enable isolated testing of the start handler by redirecting module imports to test mocks.
packages/start-server-core/tests/createStartHandler.test.ts (2)
19-33: LGTM! Core test validates 404 JSON response.This test correctly validates the main fix: routes without a GET handler now return 404 JSON instead of 200 OK with the HTML shell.
35-80: LGTM! Comprehensive method resolution coverage.The test suite thoroughly validates POST handling, HEAD→GET mapping, HEAD 404 behavior, and ANY handler fallback scenarios.
packages/start-server-core/tests/mocks/router-entry.ts (1)
5-30: LGTM! Minimal router mock for testing.The fake router provides all necessary properties to exercise the handler resolution logic without requiring the full router implementation.
|
View your CI Pipeline Execution ↗ for commit e3ccddd
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/start-server-core/tests/createStartHandler.test.ts (4)
19-32: Test correctly validates 404 JSON response for missing GET handler.The assertions comprehensively verify that API routes without a matching method return proper 404 JSON instead of the HTML shell.
Optional: Simplify line 31 assertion.
The pattern
.toLowerCase().startsWith('<!doctype html>')).toBe(false)works but could be more direct:- expect(txt.toLowerCase().startsWith('<!doctype html>')).toBe(false) + expect(txt.toLowerCase()).not.toMatch(/^<!doctype html>/i)Or validate as proper JSON:
- expect(txt.toLowerCase().startsWith('<!doctype html>')).toBe(false) + const json = JSON.parse(txt) + expect(json).toHaveProperty('error', 'Not Found')
46-55: Enhance HEAD request assertions.The test correctly verifies the 404 status, but HEAD responses have specific characteristics that should be validated.
Add assertions to verify the response body is empty and the content-type indicates JSON:
expect(res.status).toBe(404) + expect(res.headers.get('content-type')).toMatch(/application\/json/i) + const body = await res.text() + expect(body).toBe('') // HEAD responses should have empty bodies
57-66: Verify HEAD response has empty body.The test correctly checks that HEAD uses the GET handler, but should verify HEAD-specific behavior.
HEAD requests should return the same headers as GET but with an empty body (per HTTP specification):
expect(res.status).toBe(200) + const body = await res.text() + expect(body).toBe('') // HEAD responses must have empty bodies per HTTP spec
18-79: Optional: Consider additional test cases for completeness.The test suite covers the core PR objectives well. For more comprehensive coverage, consider adding:
- GET request with GET handler defined (happy path baseline)
- GET request with ANY handler but no GET (validates ANY serves as fallback for GET)
These would provide fuller documentation of expected behavior but aren't critical for validating the fix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/start-server-core/src/createStartHandler.ts(1 hunks)packages/start-server-core/tests/createStartHandler.test.ts(1 hunks)packages/start-server-core/tests/mocks/injected-head-scripts.ts(1 hunks)packages/start-server-core/tests/mocks/router-entry.ts(1 hunks)packages/start-server-core/tests/mocks/start-entry.ts(1 hunks)packages/start-server-core/tests/mocks/start-manifest.ts(1 hunks)packages/start-server-core/vite.config.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/start-server-core/tests/mocks/router-entry.ts
- packages/start-server-core/tests/mocks/start-entry.ts
- packages/start-server-core/vite.config.ts
- packages/start-server-core/tests/mocks/start-manifest.ts
- packages/start-server-core/src/createStartHandler.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-02T16:16:24.898Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Applied to files:
packages/start-server-core/tests/createStartHandler.test.ts
🧬 Code graph analysis (1)
packages/start-server-core/tests/createStartHandler.test.ts (2)
packages/start-server-core/src/createStartHandler.ts (1)
createStartHandler(54-341)packages/start-server-core/tests/mocks/router-entry.ts (1)
currentHandlers(3-3)
🔇 Additional comments (5)
packages/start-server-core/tests/mocks/injected-head-scripts.ts (1)
1-1: LGTM! Simple and appropriate test mock.The empty string export is suitable for test scenarios where injected head scripts don't need actual content.
packages/start-server-core/tests/createStartHandler.test.ts (4)
5-13: Good test infrastructure.The
spaFallbackandmakeApphelpers provide clean, reusable test setup that accurately simulates the SPA fallback behavior being tested against.
14-16: Proper test isolation.Clearing
currentHandlersbefore each test ensures tests don't interfere with each other.
34-44: Test correctly validates POST handler execution.The assertions properly verify that a defined POST handler is invoked and returns the expected response.
68-78: Test correctly validates ANY handler fallback.The assertions properly verify that the ANY handler serves as a catch-all for methods without specific handlers.
1 participant
Fixes #5473:
API routes without a GET handler returned 200 + SPA shell instead of 404 JSON.
Previously:
/api/xyzwhere only aPOSThandler existed returned 200 OK with the HTML shell.Now:
ANYfallback) correctly return 404 JSON ({ "error": "Not Found" }).HEADrequests behave correctly (empty 404 if noGEThandler).Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.